In principle, this could be a property as well.

Since it is sometimes quite subjective what could a property, I personally adopted the criterion that a method without parameters (other than self) is always a suitable property. The complicate part might be finding a suitable name :P

I was expecting this comment, however I was not sure because trim() (the next function of PulseSequence) is a very similar case and you did not make it a property. I'd say function makes a bit more sense to me for both these cases, because there is some operation (compilation) involved, but I would also be fine with properties.

Oh, you're definitely right. I could make it .trimmed.

But yes, it seems neat also as a function, because we know it's doing something (while from a property you generally expect to do little). Problem is that "something" is ill-defined (in a sense, each property does something, otherwise it would be an attribute), that's why the criterion above...

I wonder whether at this level it is just simpler to preprocess the sequence, and replace Align by a plain list of channels, while replacing the channel e.g. with an empty string, or the "align" string. And then, if channel_id == "align" you do qua.align(*pulse).
It's not that elegant from the point of view of the types (though it could be made more elegant - but it's not needed), still it should be much simpler (and you avoid iterating the whole sequence every time that you encounter an align, also because you know they are supposed to be consecutive, otherwise it would be a bug).

It's not that elegant from the point of view of the types (though it could be made more elegant - but it's not needed), still it should be much simpler (and you avoid iterating the whole sequence every time that you encounter an align, also because you know they are supposed to be consecutive, otherwise it would be a bug).

Indeed, my main issue with this is that it is not that elegant and may require more code from the driver side, like having a QM internal sequence, while I am trying to rely on qibolab primitives as much as possible. Also, the solution with the set shouldn't be much overhead in terms of performance and it is also more localized, just a few additional lines of code in a single place. The preprocess would probably have to happen somewhere else and copy the whole sequence, just to handle Align which is a very special and potentially rare case.

The set is not the problem, but rather

I was not proposing a separate sequence, just a single (inelegant) function doing:

def process_aligns(sequence: PulseSequence) -> list[Union[tuple[ChannelId, PulseLike], tuple[Literal["align"], set[ChannelId]]]]:
    new = []
    align_channels = set()
    def align():
        if len(align_channels) > 0:
            new.append(("align", align_channels))
            align_channels.clear()
    for (ch, p) in sequence:
        if isinstance(p, Align):
            align_channels.add(ch)
        else:
            align()
            new.append((ch, p))
    align()
    return new

You can tell it's not elegant from the return type (though it could be written better than that), but it requires a single iteration, without any QM-specific object (it could even be part of the sequence API...).

However, as I said, it's negligible. I'm just obsessed...

I see, thanks for the clarification. That is fine with me and I could add it to the code. Maybe a simplification would be to use the actual Align object in the tuple, instead of Literal["align"] and have the following return type:

list[tuple[Union[ChannelId, set[ChannelId]], PulseLike]]

It isn't much simpler, it's just that the second tuple element is always PulseLike. We could even get rid of the Union if we convert the first element to be a list or set always, with single element for non-aligns.

My issue with all these solutions is that, although they are more efficient when Aligns are present, they are slightly less efficient when Aligns are not present. For example in this case we'd have to copy the sequence even when Align is not present. We could check and only call process_aligns if they are actually present, but even this check would be worst case O(n) (n=len(sequence)) since we'd have to check the whole sequence. In summary, the proposed solution would be O(2n) in all cases, while the current implementation is O((m + 1)n) (m=number of aligns), so if this is correct it depends on what we expect more often. For example the current compiler doesn't use Align (and most qibocal routines will not either) so if we optimize for circuits the current implementation may actually be better in most cases. Of course negligibly better, but since we are discussing this anyway...

Yes, in practice the trade-off is even less clear, because the operations involved are not all equivalent: in one case you're just copying some Align (though iterating more PulseLike, if many Align are present), but the other is possibly doing fewer operations, but always copying the whole sequence. So, if copying were much more expensive than iterating, we had a winner...

In any case, as I said, it's just for the sake of discussion. The truth is that this part is not the performance bottleneck.
(already loading the whole platform, creating many objects, and parsing the whole parameters.json, irrespectively of the sequence, should be much more relevant - and still negligible, since most of the execution time in current experiments will be always the actual runtime on the device, and then the networking required)

I guess this is to avoid baking. What happens for the values covered multiple times?

And isn't there a // 4 already in the normalizing function?

For now this is not related to baking, but it will most likely need to be modified when baking is implemented.

Basically, the reason we need the // 4 in case_ is because it also exists in the normalizing function. In particular, the loop variable (parameters.duration) will take values from an array that was normalized with // 4, while the uploaded waveforms are enumerated with the original array, so we have to divide the enumerator when we do the matching, in order to properly match with the loop variable. We could drop the normalization in both places, however then we would need to make sure to normalize when delays are involved in the same sweeper (eg. Rabi length sweeps the duration of drive pulse and readout delay with the same sweeper), because qua.wait expects clock cycles (=4ns).

Anyway, this will probably change when baking (=supporting pulses with 1ns resolution, the best we can currently do) is implemented, so I would postpone until then, as I am planning to do it right after this PR anyway, together with the other issues from #969. Otherwise, I could push baking directly here, as I don't expect it to be very complicated.

Ok, so I get that if you're receiving as values [4, 5, 6, 7] you'll sweep four elements, but the value will always be the same one.

For as long as it is documented, for me it's perfectly acceptable.

Indeed, these values are not supported yet since baking is not implemented, so only waveforms with length that is multiple of 4 are supported (and >16). I am not sure what will be the exact behavior as I haven't tried, but I would guess that an error will appear if we try to upload a waveform of these lengths. In any case, I will implement this before we release 0.2 so it should be fine. For now I added a reference to this discussion in #969.

One additional point to check is what happens to the interpolated sweep in this case. I guess for these lengths it will fail because they are very short, however for something like [20, 21, 22, 23] it may not fail and actually return four overlapping points. If that happens we may want to raise an error ourselves and prompt to use non-interpolated sweep (which will support baking).